Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix native fetch immutable headers issue #9693

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

brophdawg11
Copy link
Contributor

Fixes #9691

Copy link

changeset-bot bot commented Jul 2, 2024

🦋 Changeset detected

Latest commit: 9d6bbf9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/testing Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/serve Patch
create-remix Patch
remix Patch
@remix-run/css-bundle Patch
@remix-run/eslint-config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 linked an issue Jul 2, 2024 that may be closed by this pull request
parentId: "root",
index: true,
loader() {
return fetch(`https://remix.run/docs/en/main?_data=root`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want to keep this here but the only way to replicate this issue was to actually get a legit Response from a fetch call which is why we didn't run into this before. Thoughts on how to best prevent a regression in this area without spamming our prod server?

@@ -360,12 +360,12 @@ async function handleDataRequest(

// Mark all successful responses with a header so we can identify in-flight
// network errors that are missing this header
response.headers.set("X-Remix-Response", "yes");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this response is a direct undici fetch Response, then the headers are immutable and we can't change them. I didn't want to clone the response in 100% of scenarios since this isn't an issue with return json() and alternatives, so I am only cloning if this operation throws the immutable error

@brophdawg11 brophdawg11 merged commit d6bc2ba into dev Jul 3, 2024
5 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/fix-native-fetch-headers branch July 3, 2024 13:40
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jul 3, 2024
Copy link
Contributor

github-actions bot commented Jul 4, 2024

🤖 Hello there,

We just published version 2.10.2-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

github-actions bot commented Jul 4, 2024

🤖 Hello there,

We just published version 2.10.2 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Jul 4, 2024
@myfrontpocket
Copy link

myfrontpocket commented Jul 9, 2024

So, I think this is the same issue here: #9212

I've updated to 2.10.2, and this does restore the previous behavior, with the exception that returning the error response results in a framework exception (that I suppose would be caught by an ErrorBoundary). Shouldn't this only happen if we throw the response rather than return it?

const response = await fetch('some-url.com')

if (!response.ok) {
  return response; // this acts a throw now
}

// continue processing...

This code before would return as loader/action data, but now it does not 😢

@brophdawg11
Copy link
Contributor Author

I do not see that behavior. Please open a ne wissue with a reporduction and we can take a look. But my quick attempt works as expected:

// routes/page.tsx
import { useLoaderData } from "@remix-run/react";

export async function loader() {
  const response = await fetch("http://localhost:5173/resource");

  if (!response.ok) {
    return response;
  }

  return { message: "from loader" };
}

export default function Comp() {
  const data = useLoaderData();
  return <p>data: {data?.message}</p>;
}

// routes/resource.tyx
import { json } from "@remix-run/node";

export function loader() {
  return json({ message: "from resource route" }, { status: 500 });
}

@myfrontpocket
Copy link

myfrontpocket commented Jul 9, 2024

I do not see that behavior. Please open a ne wissue with a reporduction and we can take a look. But my quick attempt works as expected:

// routes/page.tsx
import { useLoaderData } from "@remix-run/react";

export async function loader() {
  const response = await fetch("http://localhost:5173/resource");

  if (!response.ok) {
    return response;
  }

  return { message: "from loader" };
}

export default function Comp() {
  const data = useLoaderData();
  return <p>data: {data?.message}</p>;
}

// routes/resource.tyx
import { json } from "@remix-run/node";

export function loader() {
  return json({ message: "from resource route" }, { status: 500 });
}

Hmmm... is it the same for an action function? Not sure if it'll make a difference. This is where I'm currently seeing it happen

Nevermind, it is in a loader where I'm seeing this behavior. I'll investigate my code further and see if I can figure it out. Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loader error on data request proxying raw fetch response
3 participants